Skip to content

Read/write refs on this #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 21, 2017
Merged

Read/write refs on this #125

merged 3 commits into from
Nov 21, 2017

Conversation

ethul
Copy link
Contributor

@ethul ethul commented Nov 19, 2017

Fixes #123

Proposal to resolve the issue due to the refs object being frozen.

I am open to other ideas on this. Any feedback is most welcome!

@paf31 @coot

@paf31
Copy link
Contributor

paf31 commented Nov 19, 2017

👍 If it works, it's good with me!

@coot
Copy link

coot commented Nov 19, 2017

It looks good to me too. Maybe we could type check that one is not overwriting a life cycle method.

Using the `ref` prop writes the ref to the `this.refs` object. However,
setting a ref with a callback is not allowed to write to the frozen
`this.refs`. It writes directly to `this` instead.
@ethul
Copy link
Contributor Author

ethul commented Nov 20, 2017

Thanks for taking a look!

It turns out that react will still write a ref to this.refs when the ref prop is used (i.e., when passing a string to name the ref). I've reverted the change to getRefs so that this.refs is still accessible. Would adding some documentation on this be the best way to go?

@coot I think doing a check is a good call. Are you suggesting to check directly in the JS?

@coot
Copy link

coot commented Nov 20, 2017

I was suggesting to do that on the type level.

@ethul
Copy link
Contributor Author

ethul commented Nov 20, 2017

Gotcha. I am wondering if maybe releasing this change as a patch release could be done first. Then we can introduce the type change as a separate PR as a breaking change?

@coot
Copy link

coot commented Nov 20, 2017

Yes, I think that's fine.

@ethul ethul merged commit f2253e3 into master Nov 21, 2017
@ethul ethul deleted the issue-123 branch November 21, 2017 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Refs object is not extensible
3 participants